Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more detail error messages when installing with some components has failed. #1595

Merged
merged 2 commits into from
Jan 7, 2019
Merged

Add more detail error messages when installing with some components has failed. #1595

merged 2 commits into from
Jan 7, 2019

Conversation

h-michael
Copy link
Contributor

Error message that we get when installing with some components failed may seem unkind for beginners.
Many users create issue at component's repository.
For example,
rust-lang/rls#1186
rust-lang/rls#641
rust-lang/rustfmt#3244
rust-lang/rust-clippy#2869

So I might want to add more information.
(But there may be more suitable message.)

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Over-all the concept is probably good, though I worry about directing people to the rust-toolstate page directly.

I wonder if we should consider different messages depending on the channel selected. If this is nightly, the message you suggest would be suitable, but if this is stable or beta, that suggests something less good going on which might benefit from a different kind of message.

let _ = write!(
buf,
"some components unavailable for download: {}\n\
if you want these components, please install and use latest successful build version, \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing a 'the' between 'use' and 'latest'

buf,
"some components unavailable for download: {}\n\
if you want these components, please install and use latest successful build version, \
which you can find out at https://rust-lang-nursery.github.io/rust-toolstate, like this.\n\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd drop the 'out' replace the ',' with '.' and change 'like this.' to '\nTo do this, run something like:\n'

which you can find out at https://rust-lang-nursery.github.io/rust-toolstate, like this.\n\
rustup install nightly-2018-12-27",
cs_str
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments apply here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you pull the message out into a const please

"some components unavailable for download: 'rustc', 'cargo'\n\
if you want these components, please install and use latest successful build version, \
which you can find out at https://rust-lang-nursery.github.io/rust-toolstate, like this.\n\
rustup install nightly-2018-12-27",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments apply here.

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with the requested changes

let _ = write!(
buf,
"some components unavailable for download: {}\n\
if you want these components, please install and use latest successful build version, \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if you want these components, please install and use latest successful build version, \
if you require these components, please install and use the latest successful build, \

buf,
"some components unavailable for download: {}\n\
if you want these components, please install and use latest successful build version, \
which you can find out at https://rust-lang-nursery.github.io/rust-toolstate, like this.\n\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
which you can find out at https://rust-lang-nursery.github.io/rust-toolstate, like this.\n\
which you can find at https://rust-lang-nursery.github.io/rust-toolstate, for example.\n\

which you can find out at https://rust-lang-nursery.github.io/rust-toolstate, like this.\n\
rustup install nightly-2018-12-27",
cs_str
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you pull the message out into a const please

@h-michael
Copy link
Contributor Author

@nrc, @kinnison thanks for your review.

Could you pull the message out into a const please

How can I use write! macro with const literal ?

@nrc
Copy link
Member

nrc commented Jan 7, 2019

I was thinking you would use something like write!(buf, "some components unavailable for download: {}\n{}", ..., TOOLSTATE_MSG) where TOOLSTATE_MSG is the const).

@h-michael
Copy link
Contributor Author

@nrc I see, thanks!

@h-michael
Copy link
Contributor Author

@nrc I've fixed.
But test is failed. This may be caused by #1547.

@h-michael
Copy link
Contributor Author

@nrc Fixed and passed CI

@nrc
Copy link
Member

nrc commented Jan 7, 2019

Thanks!

@nrc nrc merged commit be16639 into rust-lang:master Jan 7, 2019
@h-michael h-michael deleted the more-detail-error-msg branch January 8, 2019 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants